APM Review Panel: ship_with_followups

PR #1292 fixes a silent enterprise auth mis-route that blocked *.ghe.com marketplace installs -- a targeted, trust-positive fix with clean security posture and strong unit coverage.

cc @edenfunf -- a fresh advisory pass is ready for your review.

This PR closes a real, user-reported enterprise regression (#1285): marketplace plugins sourced from *.ghe.com hosts were having their canonical silently left bare, causing DependencyReference.parse to default to github.com credentials and fail auth against the enterprise host. Auth-expert and supply-chain-security both confirm the fix is auth-correct and that the host injected into canonical is sourced from the trusted MarketplaceSource registry config -- never from manifest content. The triple gate (dep_ref is None + _is_in_marketplace_source + is_github_hostname excluding github.com) eliminates token cross-contamination risk. Nine unit tests including real DependencyReference.parse boundary assertions give HIGH confidence on the happy path.

Panel signals converge tightly on two follow-up clusters: (1) CHANGELOG + doc surface -- oss-growth-hacker and doc-writer both flag the missing [Unreleased] Fixed entry independently; this is the single highest-value non-code item since enterprise teams who pinned workarounds for #1285 will never discover the fix without it. (2) Cross-repo GHE plugin silent mis-route -- python-architect, devx-ux, and supply-chain-security all independently identify that cross-repo plugins on a *.ghe.com marketplace still emit a bare canonical and silently fall back to github.com auth. All three agree this is pre-existing behaviour, out of scope for this PR, but the absence of any user-facing warning makes it an invisible failure mode that will generate repeat support tickets. A single logger.warning (or structured install-time check) would close the UX gap without changing behaviour. The test-coverage-expert finding of a missing integration-with-fixtures regression trap is evidence-backed (outcome: missing, principles: secure-by-default + governed-by-policy) and ranks above opinion-only recommended findings; this is the right follow-up issue to open, not a blocker for a fix that already has strong unit coverage.

The auth-expert docstring gap (why GHES hosts are intentionally excluded from _needs_canonical_host_prefix) is a low-cost clarification that will prevent future developers from re-introducing the same class of bug. The cli-logging-expert finding of a missing logger.debug on the backfill path directly serves the --verbose diagnostic surface that operators will use when debugging future #1285-class issues.

Aligned with: Secure by Default -- fix ensures enterprise credentials are selected by default for *.ghe.com marketplace sources without any user config change. Portable by Manifest -- transparent to manifest authors, no apm.yml or plugin manifest changes required. Governed by Policy -- auth routing follows the AuthResolver credential hierarchy scoped correctly to the enterprise host; the missing integration-with-fixtures test means this policy contract is not yet machine-verified end-to-end.

Growth signal. This is a quiet enterprise unlock. GHE admins who hit #1285 searched release notes and found nothing -- a CHANGELOG entry with the *.ghe.com host pattern is the single highest-ROI item in this PR's follow-up list. Consider a dedicated "Enterprise" subsection under [Unreleased] as GHE fixes accumulate; it signals intentional enterprise investment to evaluators scanning the changelog before adoption decisions.

Panel summary

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

1. [OSS Growth Hacker + Doc Writer] Add CHANGELOG [Unreleased] Fixed entry for [BUG] Marketplace install fails for *.ghe.com hosts — auth resolves against github.com instead of registered host #1285 -- Convergent signal from two personas. Enterprise teams who pinned workarounds for [BUG] Marketplace install fails for *.ghe.com hosts — auth resolves against github.com instead of registered host #1285 will never discover this fix without a changelog entry. Suggested copy: "apm marketplace install on *.ghe.com hosts now uses enterprise credentials instead of silently falling back to github.com auth (closes [BUG] Marketplace install fails for *.ghe.com hosts — auth resolves against github.com instead of registered host #1285)". Zero-cost, high-enterprise-trust ROI.
2. [Test Coverage Expert] Add integration-with-fixtures regression trap for GHE marketplace auth routing -- Evidence-backed missing test (outcome: missing) on a secure-by-default + governed-by-policy surface. The full flow apm install -> resolve_marketplace_plugin -> canonical -> DependencyReference.parse -> auth credential lookup is not exercised by any test in tests/integration/. Suggested file: tests/integration/marketplace/test_ghe_marketplace_install_e2e.py.
3. [Python Architect + DevX UX Expert + Supply Chain Security Expert] Emit logger.warning for cross-repo GHE plugins that will silently mis-route auth -- Three independent panelists flag this pre-existing gap. When _is_in_marketplace_source returns False for a *.ghe.com marketplace and the canonical is bare owner/repo, the downstream 401 is invisible to the user. A single warning with a host-qualify hint converts a silent failure into an actionable error and prevents repeat [BUG] Marketplace install fails for *.ghe.com hosts — auth resolves against github.com instead of registered host #1285-class support tickets.
4. [CLI Logging Expert] Add logger.debug on the host-prefix backfill path -- The backfill block is the exact scenario operators will trace with --verbose when debugging *.ghe.com auth failures. Every comparable resolver branch emits a debug line. Suggested: logger.debug("Backfilled enterprise host '%s' onto canonical for %s@%s (auth routing #1285)", source.host, plugin_name, marketplace_name).
5. [Auth Expert] Add docstring note explaining why GHES (GITHUB_HOST) hosts are intentionally excluded from _needs_canonical_host_prefix -- GHES hosts correctly take the dep_ref path, but a developer reading the function will not know this without an explicit note. Prevents future developers from re-introducing the same class of auth-routing bug.

Architecture

classDiagram
    direction LR
    class resolve_marketplace_plugin {
        <<EntryPoint>>
        +plugin_name str
        +marketplace_name str
        +version_spec str
        +auth_resolver AuthResolver
        +returns MarketplacePluginResolution
    }
    class MarketplacePluginResolution {
        <<ValueObject>>
        +canonical str
        +plugin MarketplacePlugin
        +dependency_reference DependencyReference
    }
    class MarketplaceSource {
        <<ValueObject>>
        +host str
        +owner str
        +repo str
        +branch str
    }
    class MarketplacePlugin {
        <<ValueObject>>
        +name str
        +source str
        +version str
    }
    class DependencyReference {
        <<ValueObject>>
        +host str
        +repo_url str
        +virtual_path str
        +reference str
        +parse(canonical) DependencyReference
        +to_canonical() str
    }
    class _needs_canonical_host_prefix {
        <<Pure>>
        +canonical str
        +host str
        +returns bool
    }
    class _is_in_marketplace_source {
        <<Pure>>
        +plugin MarketplacePlugin
        +source MarketplaceSource
        +returns bool
    }
    class is_github_hostname {
        <<Pure>>
        +hostname str
        +returns bool
    }
    resolve_marketplace_plugin *-- MarketplacePluginResolution : produces
    resolve_marketplace_plugin ..> MarketplaceSource : reads
    resolve_marketplace_plugin ..> MarketplacePlugin : reads
    resolve_marketplace_plugin ..> _is_in_marketplace_source : calls
    resolve_marketplace_plugin ..> _needs_canonical_host_prefix : calls
    resolve_marketplace_plugin ..> DependencyReference : delegates parse
    _needs_canonical_host_prefix ..> is_github_hostname : delegates
    MarketplacePluginResolution o-- DependencyReference : optional
    class _needs_canonical_host_prefix:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

flowchart TD
    A(["apm install plugin@marketplace"]) --> B["resolve_marketplace_plugin()"]
    B --> C["fetch_or_cache() -- fetch marketplace manifest"]
    C --> D["resolve_plugin_source() -- build bare canonical: owner/repo/path"]
    D --> E{"_marketplace_host_needs_explicit_git_path(source.host)?"}
    E -->|"Yes - GitLab/generic"| F["dep_ref set via _gitlab_in_marketplace_dependency_reference()"]
    E -->|"No - github.com or *.ghe.com"| G["dep_ref = None, canonical stays bare"]
    F --> H{"dep_ref is None?"}
    G --> H
    H -->|"No - GitLab path"| I["skip host backfill"]
    H -->|"Yes"| J{"_is_in_marketplace_source() AND _needs_canonical_host_prefix()?"}
    J -->|"False: github.com, cross-repo, or already host-prefixed"| K["canonical unchanged"]
    J -->|"True: *.ghe.com in-marketplace -- FIX #1285"| L["canonical = source.host/canonical"]
    I --> M{"version_spec override?"}
    K --> M
    L --> M
    M -->|"Yes"| N["canonical = base#version_spec"]
    M -->|"No"| O["MarketplacePluginResolution(canonical, plugin, dep_ref)"]
    N --> O
    O --> P["DependencyReference.parse(canonical) -- routes auth to host in canonical"]
    P --> Q(["Auth at correct GHE host"])

Recommendation

Ship this PR. The fix is auth-correct, security-positive, and transparent to users -- auth-expert and supply-chain-security both confirm no token cross-contamination risk and a correctly scoped trust boundary. Unit test quality is HIGH with real DependencyReference.parse boundary assertions. The five follow-ups are all non-blocking improvements: the CHANGELOG entry is the highest-urgency item (open a follow-up commit or PR in the same release window); the integration-with-fixtures test should be tracked as a follow-up issue with the secure-by-default label; the cross-repo warning, debug log, and docstring clarification can land in the same minor release. None of these gaps were introduced by this PR, and none represent regressions from the pre-PR state.

_{This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.}

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review Panel for issue #1292 · ● 2.9M · ◷